-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calendar: Prevent attribute styles/classes from printing twice #43653
base: trunk
Are you sure you want to change the base?
Calendar: Prevent attribute styles/classes from printing twice #43653
Conversation
@@ -95,7 +95,7 @@ export default function CalendarEdit( { attributes } ) { | |||
<Disabled> | |||
<ServerSideRender | |||
block="core/calendar" | |||
attributes={ { ...attributes, ...getYearMonth( date ) } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if we remove the attributes
prop from the ServerSideRender
component, we would not be able to dynamically generate styles and markup based on attirbutes, including block support such as Dimension.
For example, #42029 uses attributes to apply a color to a table tag inside a calendar block.
I have addressed my concerns and suggestions in my comments here, but I think we should have a little more discussion about the policy for addressing the addition of support for blocks using the ServiersideRender
component.
Also, as mentioned within the comments above, there are five blocks that use the ServersideRender component, and duplicate class names might need to be resolved in all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good questions, thanks for opening the PR @ndiego and for the ideas here @t-hamano!
I was wondering if another option rather than preventing passing attributes
to the ServerSideRender
component would be to strip the style
key from the rest expansion of ...useBlockProps()
on the wrapper div
? That way we might be able to defer to the ServerSideRender
for all block style output when we're using the server side rendering in the editor.
I haven't tried it, and there could be edge cases with that idea, but just thought I'd raise it in case it seems like another viable option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a semi-related PR that selectively removed the test-decoration for the post comments count block's editor rendering (#43497) — while that one was to do with preventing style output that affects the placeholder state, we might be able to use a similar approach in this (and other blocks that use ServerSideRender
) to skip the style output at the wrapper div
level 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised the new approach in my comment here in reference to #43497. I would love to hear your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the discussion that took place in comment #43243, I think the following might be a good approach.
-
Don't pass
margin
andpadding
with block support toServersideRender
component (it is simpler to remove spacing all together than to limit it to margin alone, which simplifies the code) -
As for
className
, only give it to the block wrapperdiv
, as per this PR policy
Here is an example code:
// Attributes passed to ServerSideRender components.
const serverSideRenderAttirbutes = {
...attributes,
...getYearMonth( date ),
// Exclude additional CSS class(es).
className: undefined,
// Exclude spacing block support.
style: {
...attributes.style,
spacing: undefined,
},
};
return (
<div { ...blockProps }>
<Disabled>
<ServerSideRender
block="core/calendar"
attributes={ serverSideRenderAttirbutes }
/>
</Disabled>
</div>
);
I think we shuldn't use omit
, etc, because I found that #17025 is trying to move away from Lodash.
This code also takes into account the spacing support that will be added in #43654. However, we will need to brush up on this code for color support, which will be added in #42029.
@ndiego
@andrewserong
We are looking forward to your advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @t-hamano, sorry for the delay, been tied up with WCUS prep. I will take a look at this approach this weekend!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just closing the loop on the above comment for the omit
usage - #17025 has landed and we're no longer using Lodash in the entire repository.
Update: The following PRs related to the calendar block have been merged.
Regarding the first point, the Regarding the second point, since To land this PR, how about referencing the implementation of skipBlockSupportAttributes after rebasing with the latest trunk? Code// Don't omit textColor and backgroundColor attributes for which serialization is skipped.
const {
borderColor,
fontFamily,
fontSize,
gradient,
className,
...restAttributes
} = attributes;
// Don't omit color style for which serialization is skipped.
const { border, elements, spacing, typography, ...restStyles } =
attributes?.style || {};
return (
<div { ...blockProps }>
<Disabled>
<ServerSideRender
block="core/calendar"
attributes={ {
...restAttributes,
style: restStyles,
...getYearMonth( date ),
} }
/>
</Disabled>
</div>
); cc: @aaronrobertshaw |
Fixes #43652
What?
In the Editor, styles and classes generated by block attributes are generated twice for the Calendar block, once on the block wrapper and again on the server-side rendered component. This PR stops that from happening.
Why?
Up until recently, the Calendar block had no attributes other than
className
. Therefore, this bug was not much of an issue. However, as we start adding dimension and color support, issues emerge.In the case of spacing controls, padding and margin get applied twice. Therefore we cannot implement this functionality without first fixing this issue.
How?
Remove the attributes provided to the server-side rendered component.
Testing Instructions
div
.Note there is still an issue where the classname for the block is printed twice.
Screenshots or screencast
I added the custom class
table-class
to a Calendar block. Before this PR, it is printed twice. Afterward, only on the wrapperdiv
.Before:
After: